Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Minimize calls to session, module, and class-scoped fixtures by changing test order #3551

Closed
wants to merge 1 commit into from

Conversation

ceridwen
Copy link
Contributor

@ceridwen ceridwen commented Jun 7, 2018

This is very rough. All the debugging logging is still in, I haven't deleted the existing code I'm replacing, many tests are failing, and it's still buggy. I decided to post this early version before sinking any more time to see if my basic approach is acceptable.

I am trying to fix #3161. I tried to figure out how the current reordering code is supposed to work and failed. I couldn't even figure out what order it's supposed to produce. Instead, I want to define the right order as the one that minimizes the number of calls to session-scoped, then module-scoped, then class-scoped fixtures and otherwise keeps as much of the original order as possible. This minimization requirement induces a partial order on the tests that I need to extend to a total order with a topological sort. This is done in three steps in reorder_items(). First, I build some data structures using the already-existing code to get information about the fixtures. Second, I create a directed graph for the partial order induced by the fixtures. Third, I use the directed graph to topological-sort the tests with an iterative depth-first search.

This algorithm is not very efficient, I could make it more efficient at the cost of making the code more complicated. Alternately, the recursive implementation of topological sort is simpler and easier to understand, but will be slower because of the costliness of function calls in Python.

I suspect that some of the existing test failures are due to the topological sort not correctly preserving the initial order in some cases, that is bugs in this code. However, I suspect in some cases the order should change and the tests should be changed to match. Changing existing test orders is why I've targeted the features branch.

@ceridwen ceridwen changed the title [WIP] Minimize calls to session, module, and class-scoped fixtures [WIP] Minimize calls to session, module, and class-scoped fixtures by changing test order Jun 7, 2018
@RonnyPfannschmidt
Copy link
Member

im currently traveling, in case i forget wne im back home please shoot me a mail/ping me

@ceridwen ceridwen requested a review from nicoddemus June 7, 2018 22:27
@nicoddemus
Copy link
Member

Thanks @ceridwen for once again delving into this! I agree targeting the features is the way to go here.

@smarie
Copy link
Contributor

smarie commented May 20, 2019

to copy my comment in #5054, my two cents on this :

IMO all kind of optimization are highly welcome, as long as users can decide whether to use the optimized order or not. I suspect that a fair share of users might favor more simple but more readable execution order, especially when their tests do not require resources that are expensive to setup/teardown.

An extra API parameter would certainly be necessary to make this choice explicit ?

See also #3393 and #2846.

@smarie
Copy link
Contributor

smarie commented Jun 18, 2019

@ceridwen, your PR might maybe also fix #5303 - I did not check yet.

@nicoddemus
Copy link
Member

Hi @ceridwen,

It has been a long time since it has last seen activity, plus we have made sweeping changes on master to drop Python 2.7 and 3.4 support. You might give this another go now that master only supports Python 3.5+.

In order to clear up our queue and let us focus on the active PRs, I'm closing this PR for now.

Please don't consider this a rejection of your PR, we just want to get this out of sight until you have the time to tackle this again. If you get around to work on this in the future, please don't hesitate in re-opening this.

Thanks for your work, the team definitely appreciates it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants